Skip to content

[IPPInAppFeedback] UI: Payments banner placement #8532

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 13 commits into from
Closed

[IPPInAppFeedback] UI: Payments banner placement #8532

wants to merge 13 commits into from

Conversation

@iamgabrielma
Copy link
Contributor

@iamgabrielma iamgabrielma commented Jan 3, 2023 •
edited
Loading

Part of #8530

Description

This PR adds the in-app feedback banner in the Payments view, as well as the buttons to launch a survey and dismiss the banner.

At the moment the survey is only dismissible when tapping on "dismiss" (not upon survey completion), and will show up again each time we navigate to the Payments view.

TODO:

  • Design has changed. Needs to be re-implemented.
  • Dismissal and show me later/don't show again UI will be added on a different PR. Most probably using FeatureAnnouncementCardView

Changes

  • We reuse the existing Notice component to create a feedback notice banner in the Payments View.
  • Extended Notice to accept a new noticeTappedHandler handler noticeTappedHandler, so we can use 2 different behaviours: When the notice is tapped (show survey), and when the action button is tapped (dismiss notice).
  • Added a new IPPFeedback case to SurveyViewController which will launch and manage the Survey launched via the tapping the notice
  • Added a temporary testing survey link ( https://automattic.survey.fm/woo-app-ipp-in-app-feedback-testing ), this will dynamically change in future PRs based on certain conditions.

Testing instructions

  1. Go to Menu > Payments > See the notice on the bottom of the screen
  2. Tap the notice, a WebView showing a survey form will shop up. Scroll down and tap "Finish Survey", a "Feedback Sent" view will be shown and you'll be redirected to the Payments page.
  3. Navigate to a different View, then navigate to Menu > Payments again.
  4. Tap on "X" and see the banner disappear.

Screenshots

Feedback Banner Give Feedback Feedback Sent

@iamgabrielma iamgabrielma added type: enhancement A request for an enhancement. feature: mobile payments Related to mobile payments / card present payments / Woo Payments. labels Jan 3, 2023
@iamgabrielma iamgabrielma mentioned this pull request Jan 3, 2023
Closed
16 tasks
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 3, 2023 •
edited
Loading

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8532-6d73fe6 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@iamgabrielma iamgabrielma marked this pull request as ready for review January 4, 2023 06:05
@iamgabrielma iamgabrielma added this to the 11.8 milestone Jan 4, 2023
@iamgabrielma iamgabrielma requested a review from toupper January 4, 2023 07:39
@toupper
Copy link
Contributor

toupper commented Jan 5, 2023

Thanks for opening this PR @iamgabrielma. I added a couple of comments, and since testing might change depending on how the comments are addressed, I will wait for that before testing.
Apart from that, it might be more convenient to name the branch after the issue we have, to make it easier to link it (issue/8530-banner-placement). It can even be easier to split that big issue into smaller ones, but that's more your opinion.

toupper
toupper reviewed Jan 5, 2023
View reviewed changes
WooCommerce/Classes/System/WooConstants.swift Outdated
///
#if DEBUG
case inAppFeedback = "https://automattic.survey.fm/woo-app-general-feedback-test-survey"
case IPPinAppFeedback = "https://automattic.survey.fm/woo-app-ipp-in-app-feedback-testing"
Copy link
Contributor

@toupper toupper Jan 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have two inApp feedbacks, the naming looks a bit confusing to me. To make it clearer I would simply name it
generalFeedback and
IPPFeedback
We can omit the inApp prefix, we know that the feedback is triggered from the app.

Copy link
Contributor Author

@iamgabrielma iamgabrielma Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, changed these at 34b894e and 7a13d16

@iamgabrielma iamgabrielma modified the milestones: 11.8, 11.9 Jan 6, 2023
@iamgabrielma iamgabrielma added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Jan 9, 2023
@iamgabrielma iamgabrielma removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Jan 10, 2023
@iamgabrielma
Copy link
Contributor Author

iamgabrielma commented Jan 10, 2023 •
edited
Loading

Thanks for the review @toupper !

Due to design changes on this feature I've redone the banner to use a Notice instead, I updated the PR description, testing instructions, and screenshots with the new information. Here's also the design proposal, and how it looks right now when building the PR:

Design proposal Current design
Screenshot 2023-01-10 at 13 21 50

If this seems a good approach, I'll polish the UI further, as I may need to create a new Notice component that looks closer to what we need. I found other components in the codebase that may be helpful, like PermanentNotice, but while that one resolves some of the problems of using Notice, it also brings new ones. Happy to get any pointer if there's a better ones to use, or a better way to handle these.

it might be more convenient to name the branch after the issue we have, to make it easier to link it (issue/8530-banner-placement). It can even be easier to split that big issue into smaller ones, but that's more your opinion.

Yes, that was a mistake on my end when pushing the branch 😅 , the rest of issues are labeled and tracked in a better way.

@iamgabrielma iamgabrielma added the status: draft This is a draft, still need more work but can be reviewed and commented if asked. label Jan 12, 2023
@peril-woocommerce
Copy link

peril-woocommerce bot commented Jan 12, 2023

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@iamgabrielma iamgabrielma modified the milestones: 11.9, 12.0 Jan 12, 2023
@iamgabrielma
Copy link
Contributor Author

iamgabrielma commented Jan 16, 2023

Closing this issue due to modifications to scope ( pdfdoF-20T-p2 ), as we won't be showing this banner anymore in the Payments view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@toupper toupper toupper left review comments

Assignees

No one assigned

Labels

feature: mobile payments Related to mobile payments / card present payments / Woo Payments. status: draft This is a draft, still need more work but can be reviewed and commented if asked. type: enhancement A request for an enhancement.

Projects

None yet

Milestone

12.0

Development

Successfully merging this pull request may close these issues.

4 participants

@iamgabrielma @wpmobilebot @toupper